Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: (payments v3) Implement temporal specific error handling in connectors #109

Merged
merged 17 commits into from
Oct 2, 2024

Conversation

laouji
Copy link
Contributor

@laouji laouji commented Sep 30, 2024

ENG-1371

This PR only covers plugins that use the httpwrapper. I'll get Ayden and Stripe in a different PR since this one is big enough.

@laouji laouji force-pushed the feat/payments-v3-temporal-errors branch 4 times, most recently from 298336b to 4bfed82 Compare October 1, 2024 11:17
@laouji laouji marked this pull request as ready for review October 1, 2024 11:27
@laouji laouji requested a review from a team as a code owner October 1, 2024 11:27
Copy link
Contributor

@gfyrag gfyrag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General report : type assertions on error feels like a bad practice for me. I would use errors.Is and implements the required methods.

Also, this kind of code is lossy :
return nil, fmt.Errorf("failed to make payout, status code %d: %w", statusCode, err)
The original error is lost. It prevent a client from being capable to unwrap the errors.

@laouji
Copy link
Contributor Author

laouji commented Oct 1, 2024

@gfyrag If you don't like type assertions let's discuss what some other options are?

My general idea was to keep logic related to Temporal as close to the activity definitions as possible. I wanted to avoid having any logic related to temporal implemented in the client packages themselves since they should just be concerned with communicating with the various APIs.

What I set out to achieve in this PR was:

  1. API clients can return error interface without knowledge of temporal
  2. a small subset of those errors, upon being unwrapped can be converted into temporal "NoRetry" errors

I suppose instead of a having an abstracted models.PluginError type I could create a generic function like

func toTemporalError(err error) *temporal.ApplicationError {
  if errors.Is(err, SomeErr) {
  } else if...
}

Is that really nicer than a quick type assertion though?

return nil, fmt.Errorf("failed to make payout, status code %d: %w", statusCode, err)
The original error is lost. It prevent a client from being capable to unwrap the errors.

How so? the %w embeds the error in a way that allows it to be unwrapped by errors.Is() later. Actually I am relying on that mechanic to be able to detect http 4xx statuses.

@laouji laouji force-pushed the feat/payments-v3-temporal-errors branch from 4bfed82 to fe4567b Compare October 1, 2024 12:31
@laouji laouji force-pushed the feat/payments-v3-temporal-errors branch from fe4567b to a60b70b Compare October 1, 2024 16:12
@laouji laouji force-pushed the feat/payments-v3-temporal-errors branch from 2b92a2f to 5dd55f0 Compare October 2, 2024 08:07
@laouji laouji requested a review from gfyrag October 2, 2024 08:30
@laouji laouji requested review from paul-nicolas and Dav-14 October 2, 2024 08:30
@laouji
Copy link
Contributor Author

laouji commented Oct 2, 2024

@gfyrag I removed the type assertion in favour of a simple function that uses errors.Is() to determine non-retryable errors as we discussed. I also added some regression tests which should fail if the plugin functions are refactored in a way which alters the errors they return.

@paul-nicolas
Copy link
Contributor

I'am adding a comment here: everytime you do a c.httpClient.Do(req, &res, nil) which returns specific errors depending on the status code, you should do an errors.Wrap to keep the specific error instead of creating a new one

@paul-nicolas
Copy link
Contributor

I'am adding a comment here: everytime you do a c.httpClient.Do(req, &res, nil) which returns specific errors depending on the status code, you should do an errors.Wrap to keep the specific error instead of creating a new one

My bad, now the idiomatic way to do it is with the %w !

@laouji laouji merged commit 9c1ca69 into feat/payments-v3 Oct 2, 2024
6 checks passed
@laouji laouji deleted the feat/payments-v3-temporal-errors branch October 2, 2024 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants